[fix](be) Validate WAL replay column types - #66230
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: WAL replay reused a deserialized source column with the target plan's type metadata without validating that the physical and logical types matched. A schema change could therefore expose an INT column as STRING and trigger a fatal cast during projection. Validate the deserialized WAL column, its compatibility with the replay target, and exact logical type equality before inserting it into the destination block. Type mismatches now return a retryable error with WAL and column context instead of reaching the fatal cast. ### Release note None ### Check List (For Author) - Test: Not run (compilation and tests explicitly excluded by request) - Behavior changed: Yes (invalid WAL replay type mappings return an error instead of aborting the BE) - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improves WAL replay robustness by validating that deserialized WAL columns are physically compatible with the replay target and that logical types match, preventing fatal casts during projection.
Changes:
- Validate deserialized WAL column integrity via
check_type_and_column_match(). - Validate replay-target compatibility by checking physical column/type match and exact logical type equality.
- Enrich mismatch errors with WAL path, slot metadata, and source/target column context.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto& source_column = src_block.get_by_position(pos); | ||
| const auto& target_column = output_block_columns[index]; | ||
| auto source_status = source_column.check_type_and_column_match(); | ||
| if (!source_status.ok()) { | ||
| return Status::InternalError( | ||
| "Invalid WAL column while replaying WAL {}: slot={} (unique_id={}), " | ||
| "source_position={}, error={}", | ||
| _wal_path, slot_desc->col_name(), slot_desc->col_unique_id(), pos, | ||
| source_status.to_string()); | ||
| } | ||
| dst_block.insert(index, ColumnWithTypeAndName(std::move(column_ptr), | ||
| output_block_columns[index].type, | ||
| output_block_columns[index].name)); | ||
|
|
||
| ColumnWithTypeAndName replay_column(source_column.column, target_column.type, | ||
| target_column.name); |
| if (!source_column.type->equals(*target_column.type)) { | ||
| return Status::InternalError( | ||
| "WAL replay logical type mismatch: wal={}, slot={} (unique_id={}), " | ||
| "source_position={}, source_name={}, source_type={}, target_name={}, " | ||
| "target_type={}", | ||
| _wal_path, slot_desc->col_name(), slot_desc->col_unique_id(), pos, | ||
| source_column.name, source_column.type->get_name(), target_column.name, | ||
| target_column.type->get_name()); | ||
| } |
| if (!replay_status.ok()) { | ||
| return Status::InternalError( | ||
| "WAL replay column type mismatch: wal={}, slot={} (unique_id={}), " | ||
| "source_position={}, source_name={}, source_type={}, source_column={}, " | ||
| "target_name={}, target_type={}, error={}", | ||
| _wal_path, slot_desc->col_name(), slot_desc->col_unique_id(), pos, | ||
| source_column.name, source_column.type->get_name(), | ||
| source_column.column->get_name(), target_column.name, | ||
| target_column.type == nullptr ? "null" : target_column.type->get_name(), | ||
| replay_status.to_string()); | ||
| } |
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29461 ms |
TPC-DS: Total hot run time: 177894 ms |
ClickBench: Total hot run time: 25.16 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes.
Goal and scope: This PR is narrowly focused on preventing a fatal WAL projection cast by validating deserialized source/target columns. The one-file change was reviewed end to end; no additional user focus was provided.
Blocking findings:
- Supported retained-ID schema evolution can be rejected permanently before the existing load CAST.
- Distinct AGG_STATE signatures/layouts can pass the new checks and be relabeled.
- New retryable errors bypass authenticated rollback; wait mode can exhaust retries, move the WAL to
tmp, and notify without a failure value. - No enabled test exercises the serialized compatibility boundary and transaction/retry lifecycle.
Critical checkpoints:
- Data correctness/schema compatibility: The blockers above cover retained-ID evolution and parameterized aggregate-state storage. Nullable normalization and stable-ID drop/rename/reorder mapping are otherwise consistent; no WAL serialization format or FE-BE protocol changes.
- Error handling/lifecycle/transactions: New statuses propagate and
InternalErroris retried, but authenticated rollback/failure signaling is broken as described inline. No changed status is silently ignored insideWalReader. - Concurrency/memory/performance: No new shared state, locks, thread entry, static/global lifetime, material allocation, or row-wise work. Checks are per-column/type-shape.
- Parallel/config/persistence/observability: Load uses the legacy scanner because v2 is disabled for loads. No new configuration, dynamic-update, EditLog, or storage-format behavior. Both replay-wait settings were audited; contextual errors and existing logs/metrics help, while wait-mode exhaustion signaling is part of MF-1.
- Tests: No builds or tests were run because this runner is review-only. The repository coverage gap is MF-2.
User focus: No additional focus; the complete PR was reviewed.
Status: Complete review of head 6b2147ec88268362bfd8fc14742621809c14b895; request changes with four inline comments.
| auto source_status = source_column.check_type_and_column_match(); | ||
| if (!source_status.ok()) { | ||
| return Status::InternalError( | ||
| "Invalid WAL column while replaying WAL {}: slot={} (unique_id={}), " |
There was a problem hiding this comment.
[P2] Add executable coverage for the mismatch-and-retry path
This changes a kernel replay path to replace a fatal projection cast with compatibility checks and retryable statuses, but the PR adds no test for that behavior. The helper test only checks hand-built columns, the WAL writer/reader test round-trips matching types, and the only legacy scanner tests are disabled/count-only. Please add enabled serialized-WAL coverage that distinguishes a supported promotion (which should reach the existing CAST) from a genuinely incompatible mapping and AGG_STATE signature/layout (which should return a contextual error), and verify authenticated rollback, WAL retention/retry, and wait-mode exhaustion signaling after a fragment failure.
| return Status::InternalError( | ||
| "Invalid WAL column while replaying WAL {}: slot={} (unique_id={}), " | ||
| "source_position={}, error={}", | ||
| _wal_path, slot_desc->col_name(), slot_desc->col_unique_id(), pos, |
There was a problem hiding this comment.
[P1] Roll back the replay transaction before requeueing this error
These new mismatch returns propagate through the fragment future, but WalTable::_handle_stream_load calls RETURN_IF_ERROR(ctx->load_status_future.get()) before its explicit rollback_txn. This native-insert path gets txn_id from the FE plan and leaves need_rollback false, so neither the callback nor StreamLoadContext's destructor rolls the failed transaction back. Normal mode only attempts an abort at the start of the next retry; with group_commit_wait_replay_wal_finish=true that abort is skipped entirely, and the same label can exhaust its retries before the WAL is moved to tmp and the waiter is notified without a failure value. Please propagate the future failure into both the returned status and ctx->status, restore internal token/auth-code authentication, and complete rollback before requeueing it.
|
|
||
| ColumnWithTypeAndName replay_column(source_column.column, target_column.type, | ||
| target_column.name); | ||
| auto replay_status = replay_column.check_type_and_column_match(); |
There was a problem hiding this comment.
[P1] Preserve the serialized source type through compatible schema changes
This target-type validation rejects retained-ID schema changes before the load scanner can cast them. For example, a light ARRAY<INT> -> ARRAY<BIGINT> change can leave an older WAL whose ColumnArray contains Int32; relabeling it as the current ARRAY<BIGINT> fails here even though FileScanner::_cast_to_input_block has the element-wise conversion. Heavy scalar changes are normally fenced by the WAL drain, but its timeout or an unavailable BE can likewise leave an old INT WAL after the current type becomes BIGINT/STRING, and retry will never make it match. Please retain source_column.type and apply a schema-compatibility decision before the existing CAST for supported same-shape conversions; shape evolution such as appended STRUCT fields also needs explicit field/default mapping because the legacy struct CAST requires equal arity.
|
|
||
| ColumnWithTypeAndName replay_column(source_column.column, target_column.type, | ||
| target_column.name); | ||
| auto replay_status = replay_column.check_type_and_column_match(); |
There was a problem hiding this comment.
[P1] Validate the complete AGG_STATE signature and storage layout
This compatibility check is too weak for AGG_STATE. If an old sum(INT) WAL survives the bounded schema-change drain after the column becomes avg(BIGINT), FE has accepted a changed AGG_STATE type, but BE DataTypeAggState::equals still compares only the dynamic type and its DataTypeFixedLengthObject checker ignores item_size. The source check, this replay check, and the later equality can therefore accept payloads with different sizes and meanings. The downstream CAST also reduces both states to DataTypeFixedLengthObject and selects the identity path, so sum bytes can be interpreted as an avg state. Please compare the full function/argument/nullability/version signature and validate the serialized layout (including fixed item size) before relabeling the column.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Problem Summary: WAL replay reused a deserialized source column with the target plan's type metadata without validating that the physical and logical types matched. A schema change could therefore expose an INT column as STRING and trigger a fatal cast during projection. Validate the deserialized WAL column, its compatibility with the replay target, and exact logical type equality before inserting it into the destination block. Type mismatches now return a retryable error with WAL and column context instead of reaching the fatal cast.